Skip to content

Conversation

@kabilar
Copy link
Member

@kabilar kabilar commented Jan 16, 2026

@kabilar kabilar requested a review from erdalkaraca as a code owner January 16, 2026 05:35
@codecov
Copy link

codecov bot commented Jan 16, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.81%. Comparing base (1fd10bb) to head (6851fa9).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2309   +/-   ##
=======================================
  Coverage   82.81%   82.81%           
=======================================
  Files          22       22           
  Lines        1693     1693           
=======================================
  Hits         1402     1402           
  Misses        291      291           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Comment on lines 915 to 916
"DiffGradientDuration": 11.0,
"DiffGradientSeparation": 15.2,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't generally use abbreviations. I would say either "DiffusionGradientDuration/Separation" or "GradientDuration/Separation". I would probably prefer the latter if the terms are unambiguous within a diffusion context and wouldn't refer to something else in another context BIDS is likely to touch on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @effigies. Just avoid any potential future confusion, I would prefer "DiffusionGradientDuration/Separation".

@ayendiki @Tinggong What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't necessarily like the word "separation", which to me implies the interval between the end of one pulse and the beginning of the other pulse. Other words to potentially use here are "interval", or "offset". I do wonder if given their prevalence in the literature, BigDelta and SmallDelta wouldn't be clearer. Also, I do believe that this is a redefinition of something that could already be captured by MixingTime: https://bids-specification.readthedocs.io/en/stable/glossary.html#mixingtime-metadata?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this as related to #2311

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not familiar with how diffusion sensitization plays out in a "double spin echo" diffusion weighted sequence, but are MixingTime and BigDelta really the same thing?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timing parameters are defined quite differently for "double" or "twice refocused" spin echo sequences, see https://onlinelibrary.wiley.com/doi/10.1002/mrm.10308. IIUC (see Fig 1 therein), there is no "BigDelta" in these sequences, and the intensity of the gradients is somehow integrated across all those small deltas, so I think that's to be handled separately, maybe in the advanced dMRI BEP.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then, the existing MixingTime is not appropriate here, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not too familiar with MixingTime but this paper declares the mixing time, gradient separation, and gradient duration as separate values:

...mixing time TM=173.0 ms, gradient separation Δ=185.8 ms and a gradient duration of δ=5.4ms.

@tsalo
Copy link
Member

tsalo commented Jan 23, 2026

I believe that some large datasets have been using their own versions of this (e.g., HBCD has LargeDelta and SmallDelta), so it would be good to get input on the naming from some folks who might be doing something similar (@mharms, @Lestropie, @arokem).

@mharms
Copy link

mharms commented Jan 23, 2026

BigDelta (rather than LargeDelta) and SmallDelta would be the commonly used colloquial terms.

What about sequences that might allow for differing deltas across the differing volumes? How would that be handled?

@mharms
Copy link

mharms commented Jan 23, 2026

Also, are BIDS timing parameters no longer all specified in seconds? (This proposal was specifying these parameters in msec).

@ayendiki
Copy link

BigDelta (rather than LargeDelta) and SmallDelta would be the commonly used colloquial terms.

What about sequences that might allow for differing deltas across the differing volumes? How would that be handled?

That's exactly our use case for our imminent data release. There are 2 competing options right now: separate text files like bvec and bval, or a sequence of numbers in the JSON file. Would love to get a consensus that involves bvec, bval, TE, δ, Δ all being handled the same way.

@mharms
Copy link

mharms commented Jan 23, 2026

Would love to get a consensus that involves bvec, bval, TE, δ, Δ all being handled the same way.

Well, the treatment of bvec and bval as separate text files is longstanding, and that presumably isn't going to change. Does the current standard support an array for TE, if the TE is changing as well?

@effigies
Copy link
Collaborator

Also, are BIDS timing parameters no longer all specified in seconds?

We have 42 fields with units s and only 3 with other time units (all ms):

󰅂 git grep 'unit: ' src/schema/objects/metadata.yaml | sed -e 's/: [ -]*/: /' | sort | uniq -c | sort -h 
      1 src/schema/objects/metadata.yaml: unit: cm
      1 src/schema/objects/metadata.yaml: unit: g/mL
      1 src/schema/objects/metadata.yaml: unit: mL
      1 src/schema/objects/metadata.yaml: unit: mL/s
      1 src/schema/objects/metadata.yaml: unit: mT.s/m
      1 src/schema/objects/metadata.yaml: unit: um
      1 src/schema/objects/metadata.yaml: unit: uT
      2 src/schema/objects/metadata.yaml: unit: arbitrary
      2 src/schema/objects/metadata.yaml: unit: cm/s
      2 src/schema/objects/metadata.yaml: unit: dB
      2 src/schema/objects/metadata.yaml: unit: MHz
      2 src/schema/objects/metadata.yaml: unit: mT/m
      3 src/schema/objects/metadata.yaml: unit: ms
      4 src/schema/objects/metadata.yaml: unit: ppm
      6 src/schema/objects/metadata.yaml: unit: degree
      6 src/schema/objects/metadata.yaml: unit: m
      8 src/schema/objects/metadata.yaml: unit: mm
     10 src/schema/objects/metadata.yaml: unit: Hz
     42 src/schema/objects/metadata.yaml: unit: s

There are two ASL metadata fields and one MRS field that use milliseconds (LabelingPulseDuration, LabelingPulseInterval, and EditPulse.PulseDuration). I don't know if there were discussions or if these flew under the radar. Possibly @HenkMutsaerts and @markmikkelsen will recall?

I think it does make sense to stick with seconds, but it's not unprecedented to use ms.

@kabilar
Copy link
Member Author

kabilar commented Jan 23, 2026

What about sequences that might allow for differing deltas across the differing volumes? How would that be handled?

Hi @mharms, I filed #2320 to handle this case. But there is some subsequent discussion in #2302 related to this that is also summarized by @ayendiki above.

@kabilar
Copy link
Member Author

kabilar commented Jan 23, 2026

Does the current standard support an array for TE, if the TE is changing as well?

Yes, TE does allow for an array of numbers. Please see Timing parameters.

@kabilar
Copy link
Member Author

kabilar commented Jan 23, 2026

Also, are BIDS timing parameters no longer all specified in seconds? (This proposal was specifying these parameters in msec).

I think it does make sense to stick with seconds, but it's not unprecedented to use ms.

Happy to switch to seconds if that is common across BIDS. I opted for milliseconds here since that is how δ and Δ (and TE) are reported in manuscripts.

@markmikkelsen
Copy link
Contributor

There are two ASL metadata fields and one MRS field that use milliseconds (LabelingPulseDuration, LabelingPulseInterval, and EditPulse.PulseDuration). I don't know if there were discussions or if these flew under the radar. Possibly @HenkMutsaerts and @markmikkelsen will recall?

I think it does make sense to stick with seconds, but it's not unprecedented to use ms.

@effigies: EditPulse.PulseDuration is always reported in ms in the MRS literature, but given the vast majority of units of time in BIDS are reported in s, I wouldn't be opposed to changing it to s.

@markmikkelsen
Copy link
Contributor

It's like with EchoTime and RepetitionTime: I personally always report them in ms in my papers, but they are reported in s in BIDS.

@markmikkelsen
Copy link
Contributor

markmikkelsen commented Jan 24, 2026

I can start a separate PR for this if needed.

@effigies
Copy link
Collaborator

No, that would break backwards compatibility. Tools could probably figure out the scale by looking at the value, but it's not worth having one interpretation in one version and another in another.

@kabilar
Copy link
Member Author

kabilar commented Jan 26, 2026

Hi all, thanks for the feedback. I have updated this pull request. Next I will update #2320.

@kabilar kabilar requested review from arokem, effigies and mharms January 26, 2026 21:54
format: participant_relative
BigDelta:
name: BigDelta
display_name: Diffusion Gradient Separation
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure where/how these "display_name" fields show up, but I wonder if it should just be "Diffusion BigDelta"?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One place it shows up is in the Glossary under the Name field. How about the following?

Suggested change
display_name: Diffusion Gradient Separation
display_name: Diffusion Big Delta (Δ)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants